Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't highlight vars with colons as keywords #670

Merged
merged 4 commits into from
Nov 24, 2023
Merged

Don't highlight vars with colons as keywords #670

merged 4 commits into from
Nov 24, 2023

Conversation

daveliepmann
Copy link
Contributor

@daveliepmann daveliepmann commented Nov 21, 2023 •
edited
Loading

Fixes #653

Changes syntax highlighting regexp for keywords to match a colon/double-colon only at the beginning of a word, not in the middle. This allows local vars like foo:bar to be highlighted correctly instead of like an unknown symbol for the part before the colon and a keyword for the rest.

For my own reference, the "\<" is per https://www.gnu.org/savannah-checkouts/gnu/emacs/manual/html_node/emacs/Regexp-Backslash.html#Regexp-Backslash

Test suite

(def foo:bar 5)
(def legal*stuff 10)
(def very*****legal 10)

;; wrong
f:bar
foo:bar
defined-in-another-ns/bar:baz
foooooo:bar

;; right
(try)
fubar
inc
legal*stuff
very*****legal
defined-in-another-ns/lorem-ipsum
:baz
::qualified
(:baz {:baz 5})
:qux/foo
::ns/name
*ns*
*assert*
clojure.core/*assert*
(*assert*)
nil

Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

  • The commits are consistent with our contribution guidelines.
  • You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
  • You've run M-x checkdoc and fixed any warnings in the code you've written.
  • You've updated the changelog (if adding/changing user-visible functionality).
  • You've updated the readme (if adding/changing user-visible functionality).

Thanks!

@daveliepmann
Don't highlight vars with colons as keywords
88f75d0 
Changes syntax highlighting regexp for keywords to match a colon/double-colon
only at the beginning of a word, not in the middle. This allows local vars like
`foo:bar` to be highlighted correctly instead of like an unknown symbol for the
part before the colon and a keyword for the rest.

Fixes clojure-emacs#653
vemv
vemv reviewed Nov 21, 2023
View reviewed changes
Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @daveliepmann , this looks like a reasonable addition!

We'd appreciate if unit tests were included.

Thanks - V

@daveliepmann
Copy link
Contributor Author

daveliepmann commented Nov 21, 2023 •
edited
Loading

Hope these suffice. Was easier than expected, thanks for the nudge

vemv
vemv approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I'll try it locally

vemv
vemv reviewed Nov 22, 2023
View reviewed changes
Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following test cases are valid Clojure so would be worth adding.

One of them would be broken, hopefully the fix won't complicate things?

:a:a ;; OK
:a:a/:a ;; the last two characters don't get keyword rendering
::a:a ;; OK
::a.a:a ;; OK

@daveliepmann
Copy link
Contributor Author

all 4 seem to work for me — the namespaced keyword path was not touched

@vemv
Copy link
Member

vemv commented Nov 22, 2023

Looks good. Mind to share a screenshot of the rendering of those 4 cases?

@bbatsov
Copy link
Member

bbatsov commented Nov 22, 2023

Out of curiosity - why do people use such symbols in the wild? I know the Clojure parser allows it (I doubt this was intentional, though), but it always seemed like an anti-pattern to me, that's why I never bothered much to support it in clojure-mode.

@daveliepmann
Copy link
Contributor Author

daveliepmann commented Nov 22, 2023 •
edited
Loading

Looks good. Mind to share a screenshot of the rendering of those 4 cases?

Screenshot 2023-11-22 at 10 23 22

The namespace looks wrong on :a:a/:a, right?

@daveliepmann
Copy link
Contributor Author

Out of curiosity - why do people use such symbols in the wild? I know the Clojure parser allows it (I doubt this was intentional, though), but it always seemed like an anti-pattern to me, that's why I never bothered much to support it in clojure-mode.

I started using it on a couple client projects because the lead dev (using Cursive) liked it. We had a part of the codebase which had standardized CRUD structure which cut across namespaces, so it made sense to have query and view vars side-by-side which included both object and verb, e.g. org:one, org:index, member:create. It made sense; the biggest drawback was syntax highlighting on my machine.

Now I mostly use it as a form of lightweight grouping, almost like a pseudo-namespace. E.g. widget:foo, widget:bar, screen:baz.

I don't see why it would be an anti-pattern. I don't know whether it was originally intentional but it is now explicitly pointed out as legal in the reader reference.

@daveliepmann
Copy link
Contributor Author

Looks good. Mind to share a screenshot of the rendering of those 4 cases?

Screenshot 2023-11-22 at 10 23 22

The namespace looks wrong on :a:a/:a, right?

I struck out my comment above because on second look,

  1. I don't think I changed this behavior since I didn't touch the namespaced keyword code path,
  2. I don't think it should work per clojure--keyword-sym-forbidden-1st-chars
  3. I think that correctly reflects the reader per "Symbols beginning or ending with ':' are reserved by Clojure." — the keyword :a:a/:a is not valid
  4. the corrected version :a:a/a works fine:
Screenshot 2023-11-22 at 18 38 51

@vemv vemv merged commit 825f9ab into clojure-emacs:master Nov 24, 2023
@vemv
Copy link
Member

vemv commented Nov 24, 2023

Thanks much!

I don't have a strong opinion for either side at the moment - we can always refine things later, particularly as indeed it's an unrelated concern.

Will cut a stable release.

@daveliepmann
Copy link
Contributor Author

Thanks 🙌

@daveliepmann daveliepmann mentioned this pull request Nov 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vemv vemv vemv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Symbols with colon in the middle get wrong syntax highlighting
3 participants
@daveliepmann @vemv @bbatsov